ARROW-8459: [Dev][Archery] Use a more recent cmake-format by kszucs · Pull Request #10571 · apache/arrow

kszucs · 2021-06-22T13:42:46Z

bump cmake-format's version to the latest one
port run-cmake-format.py script to archery
support archery lint --cmake-format format checks without reformatting the files in-place
support archery lint --cmake-format --fix for actually reformat the files
reformat the cmake files

I assume we may need tune the options a little bit, so feel free to experiment with the values defined in cmake-format.py then re-run archery-lint --cmake-format --fix.

The cmakelang package also provides a cmake-lint command which we could experiment with in the future.

github-actions · 2021-06-22T13:43:40Z

https://issues.apache.org/jira/browse/ARROW-8459

kszucs · 2021-06-22T13:50:04Z

-                       "AVX2"
-                       "AVX512"
-                       "MAX")
+  define_option_string(


I think we can alter this tabulation behavior using https://cmake-format.readthedocs.io/en/latest/configopts.html#min-prefix-chars

Could you try 32 or something?
I don't expect formatter so much but I want to reduce diff to look history easily later.

Yes, I'm going to try formatting with min-prefix-chars=32

pitrou · 2021-06-22T15:04:09Z

-    def __init__(self, cmake_format_bin):
-        self.bin = cmake_format_bin
+
+    # TODO(kszucs): check cmake_format version


I don't know if you can do it in this PR?

Wait, it seems done already below. Perhaps just remove this TODO?

Yep, thanks for spotting that!

pitrou · 2021-06-22T15:04:45Z

+            'go/**/CMakeLists.txt',
+            'java/**/CMakeLists.txt',
+            'matlab/**/CMakeLists.txt',
+        ],


We have cmake files in python as well, no?

Those haven't been checked earlier either https://github.com/apache/arrow/blob/master/run-cmake-format.py#L29
I think we could simply use a global pattern with an exclusion list rather.

We have only python/CMakeLists.txt in python/.
python/cmake_modules/ is a symbolic link to cpp/cmake_modules/.

I added python/CMakeLists.txt.

pitrou · 2021-06-22T15:05:54Z

+            'cpp/cmake_modules/FindPythonLibsNew.cmake',
+            'cpp/cmake_modules/UseCython.cmake',
+            'cpp/src/arrow/util/config.h.cmake',
+        ]


Do you know the reasons for these exclusions?

Ported from the previous script, I assume these files are vendored or generated.

cpp/cmake_modules/*.cmake in the list are imported CMake files: #9045 (comment)

We can't use cmake-format for cpp/src/arrow/util/config.h.cmake because it's not valid CMake file. It has @...@ to replace variables by CMake.

kou · 2021-06-23T02:47:24Z

+  LLVMAlt
+  REQUIRED_VARS # The first variable is used for display.
+                LLVM_PACKAGE_VERSION CLANG_EXECUTABLE LLVM_FOUND LLVM_LINK_EXECUTABLE)


Suggested change

LLVMAlt

REQUIRED_VARS # The first variable is used for display.

LLVM_PACKAGE_VERSION CLANG_EXECUTABLE LLVM_FOUND LLVM_LINK_EXECUTABLE)

LLVMAlt

# The first variable is used for display.

REQUIRED_VARS LLVM_PACKAGE_VERSION CLANG_EXECUTABLE LLVM_FOUND LLVM_LINK_EXECUTABLE)

kou · 2021-06-23T02:48:19Z

+  Arrow
+  REQUIRED_VARS # The first required variable is shown
+                # in the found message. So this list is
+                # not sorted alphabetically.
+                ARROW_INCLUDE_DIR ARROW_LIB_DIR ARROW_FULL_SO_VERSION ARROW_SO_VERSION


Suggested change

Arrow

REQUIRED_VARS # The first required variable is shown

# in the found message. So this list is

# not sorted alphabetically.

ARROW_INCLUDE_DIR ARROW_LIB_DIR ARROW_FULL_SO_VERSION ARROW_SO_VERSION

Arrow

# The first required variable is shown in the found message. So this list is

# not sorted alphabetically.

REQUIRED_VARS ARROW_INCLUDE_DIR ARROW_LIB_DIR ARROW_FULL_SO_VERSION ARROW_SO_VERSION

kou · 2021-06-23T02:51:52Z

+            'go/**/CMakeLists.txt',
+            'java/**/CMakeLists.txt',
+            'matlab/**/CMakeLists.txt',
+        ],


We have only python/CMakeLists.txt in python/.
python/cmake_modules/ is a symbolic link to cpp/cmake_modules/.

kou · 2021-06-23T02:55:06Z

+            'cpp/cmake_modules/FindPythonLibsNew.cmake',
+            'cpp/cmake_modules/UseCython.cmake',
+            'cpp/src/arrow/util/config.h.cmake',
+        ]


cpp/cmake_modules/*.cmake in the list are imported CMake files: #9045 (comment)

We can't use cmake-format for cpp/src/arrow/util/config.h.cmake because it's not valid CMake file. It has @...@ to replace variables by CMake.

kou · 2021-06-23T02:56:46Z

-                       "AVX2"
-                       "AVX512"
-                       "MAX")
+  define_option_string(


Could you try 32 or something?
I don't expect formatter so much but I want to reduce diff to look history easily later.

kszucs · 2021-06-23T15:23:51Z

@kou updated the formatting with min-prefix-chars=32, please take a look again.

kou

+1

Thanks. It looks good with min-prefix-chars=32.

kou · 2021-06-23T20:22:25Z

+            'go/**/CMakeLists.txt',
+            'java/**/CMakeLists.txt',
+            'matlab/**/CMakeLists.txt',
+        ],


This is a follow-up of apache#10571 / ARROW-8459 .

This is a follow-up of #10571 / ARROW-8459 . Closes #11112 from kou/github-actions-autotune Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

This is a follow-up of apache#10571 / ARROW-8459 . Closes apache#11112 from kou/github-actions-autotune Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

- [x] bump cmake-format's version to the latest one - [x] port `run-cmake-format.py` script to archery - [x] support `archery lint --cmake-format` format checks without reformatting the files in-place - [x] support `archery lint --cmake-format --fix` for actually reformat the files - [x] reformat the cmake files I assume we may need tune the options a little bit, so feel free to experiment with the values defined in `cmake-format.py` then re-run `archery-lint --cmake-format --fix`. The `cmakelang` package also provides a `cmake-lint` command which we could experiment with in the future. Closes apache#10571 from kszucs/update-cmake-format Authored-by: Krisztián Szűcs <szucs.krisztian@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

kszucs added 3 commits June 22, 2021 15:11

Move run-cmake-format.py to archery

3541590

Execute cmake-format --fix

fe571d5

Restore max_subargs_per_line option as max_pargs_hwrap

7e2b255

kszucs requested review from kou and pitrou June 22, 2021 13:43

github-actions Bot added Component: FlightRPC Component: Gandiva Component: C++ Component: Java Component: Parquet labels Jun 22, 2021

kszucs commented Jun 22, 2021

View reviewed changes

Approximate previous tabulation

2b1d79a

pitrou reviewed Jun 22, 2021

View reviewed changes

kszucs added 2 commits June 22, 2021 17:30

Update archery installation

c0684df

Format python/CMakeList.txt

f7584ff

github-actions Bot added the Component: Python label Jun 22, 2021

Recommend installing in editable mode

dcca09d

kou reviewed Jun 23, 2021

View reviewed changes

Comment thread .pre-commit-config.yaml

Reformat using min_prefix_chars = 32

ad2f15a

kou approved these changes Jun 23, 2021

View reviewed changes

kou closed this in 9aaf61c Jun 23, 2021

kou mentioned this pull request Jun 23, 2021

ARROW-12730: [MATLAB] Update featherreadmex and featherwritemex to build against latest Arrow C++ APIs #10305

Closed

kou added a commit to kou/arrow that referenced this pull request Sep 9, 2021

ARROW-13942: [Dev] Update cmake_format usage in autotune comment bot

5d52fa0

This is a follow-up of apache#10571 / ARROW-8459 .

kou mentioned this pull request Sep 9, 2021

ARROW-13942: [Dev] Update cmake_format usage in autotune comment bot #11112

Closed

asfimport mentioned this pull request Jun 23, 2021

[Dev][Archery] Use a more recent cmake-format #24634

Closed

Conversation

kszucs commented Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jun 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kszucs Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kszucs commented Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kszucs commented Jun 22, 2021 •

edited

Loading

kszucs Jun 22, 2021 •

edited

Loading

kszucs commented Jun 23, 2021 •

edited

Loading